fix: Add back the Fix for Pipeline variables related customer issues #3043

qidewenwhen · 2022-04-02T19:56:26Z

Issue #, if available:

Revert "fix: Fix Pipeline variables related customer issues (#2959)" #3041

Description of changes:

Add back the fix: Fix Pipeline variables related customer issues: fix: Fix Pipeline variables related customer issues #2959
Fix the issue brought by the PR above: include the deprecated JsonGet into the PipelineVariable

Testing done: integ tests

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

I have read the CONTRIBUTING doc
I certify that the changes I am introducing will be backword compatible, and I have discussed concerns about this, if any, with the Python SDK team
I used the commit message format described in CONTRIBUTING
I have passed the region in to all S3 and STS clients that I've initialized as part of this change.
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have added unit and/or integration tests as appropriate to ensure backward compatibility of the changes
I have checked that my tests are not configured for a specific region or account (if appropriate)
I have used unique_name_from_base to create resource names in integ tests (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sagemaker-bot · 2022-04-02T20:24:04Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: e7ca352
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

codecov-commenter · 2022-04-02T20:31:23Z

Codecov Report

Merging #3043 (fe2f885) into master (4fc7f2c) will decrease coverage by 0.00%.
The diff coverage is 90.32%.

❗ Current head fe2f885 differs from pull request most recent head 34628be. Consider uploading reports for the commit 34628be to get more accurate results

@@            Coverage Diff             @@
##           master    #3043      +/-   ##
==========================================
- Coverage   89.39%   89.39%   -0.01%     
==========================================
  Files         197      196       -1     
  Lines       16672    16656      -16     
==========================================
- Hits        14904    14889      -15     
+ Misses       1768     1767       -1

Impacted Files	Coverage Δ
src/sagemaker/workflow/_repack_model.py	`100.00% <ø> (ø)`
src/sagemaker/workflow/condition_step.py	`100.00% <ø> (ø)`
src/sagemaker/workflow/pipeline.py	`88.15% <60.00%> (-1.11%)`	⬇️
src/sagemaker/session.py	`70.52% <66.66%> (ø)`
src/sagemaker/estimator.py	`91.30% <100.00%> (ø)`
src/sagemaker/model.py	`92.16% <100.00%> (+0.02%)`	⬆️
src/sagemaker/parameter.py	`97.29% <100.00%> (ø)`
src/sagemaker/processing.py	`96.11% <100.00%> (+0.03%)`	⬆️
src/sagemaker/tensorflow/model.py	`90.32% <100.00%> (ø)`
src/sagemaker/tuner.py	`98.12% <100.00%> (+0.20%)`	⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fc7f2c...34628be. Read the comment docs.

sagemaker-bot · 2022-04-02T21:17:46Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: e7ca352
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2022-04-04T17:00:06Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: d069836
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2022-04-04T18:06:26Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: d069836
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2022-04-05T20:24:28Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: fe2f885
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2022-04-05T21:00:07Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: fe2f885
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2022-04-05T22:08:24Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: fe2f885
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

shreyapandit · 2022-04-07T19:14:04Z

src/sagemaker/session.py

@@ -2340,13 +2342,17 @@ def _map_training_config(
            training_job_definition["VpcConfig"] = vpc_config

        if enable_network_isolation:
-            training_job_definition["EnableNetworkIsolation"] = True
+            training_job_definition["EnableNetworkIsolation"] = enable_network_isolation


nit: Isnt this redundant, if F will never come here?

This was caught from a customer reported issue.
enable_network_isolation can be a Pipeline variable like ParameterBoolean, which is not parsed in compile time (SDK stage).
However, in such case, it will go into this line and hard code a True to it, which means the ParameterBoolean object will be lost in the Pipeline definition and accordingly users are not able to update this parameter in runtime.

shreyapandit · 2022-04-07T19:15:02Z

src/sagemaker/workflow/_repack_model.py

        dependencies (str): A space-delimited string of paths to custom dependencies.
        source_dir (str): The path to a custom source directory.
    """

    # the data directory contains a model archive generated by a previous training job
    data_directory = "/opt/ml/input/data/training"
-    model_path = os.path.join(data_directory, model_archive)
+    model_path = os.path.join(data_directory, model_archive.split("/")[-1])



Why are we changing this?

This _repack_model.py is a script which will be uploaded and executed during Pipeline execution runtime (backend), which means all pipeline variables are correctly parsed to proper type like str at that time.
So it's safe to do the str split in this script.
A corresponding change is in _utils.py below (link). As we move the str split into the _repack_model.py, we can clean up those condition check lines in _utils.py.

…ws#2959)" (aws#3041)" This reverts commit 2782f8c.

sagemaker-bot · 2022-04-07T19:52:41Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 34628be
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2022-04-07T20:34:55Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: 34628be
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2022-04-07T20:49:32Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: 34628be
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…ws#3043) * Revert "Revert "fix: Fix Pipeline variables related customer issues (aws#2959)" (aws#3041)" This reverts commit 2782f8c. * fix: Include deprecated JsonGet into PipelineVariable Co-authored-by: Dewen Qi <[email protected]>

qidewenwhen force-pushed the dev branch from e7ca352 to d069836 Compare April 4, 2022 16:32

staubhp approved these changes Apr 4, 2022

View reviewed changes

xchen909 added the component: pipelines Relates to the SageMaker Pipeline Platform label Apr 4, 2022

ahsan-z-khan requested a review from shreyapandit April 5, 2022 17:54

qidewenwhen force-pushed the dev branch from d069836 to fe2f885 Compare April 5, 2022 19:57

shreyapandit reviewed Apr 7, 2022

View reviewed changes

shreyapandit approved these changes Apr 7, 2022

View reviewed changes

Dewen Qi added 2 commits April 7, 2022 12:31

Revert "Revert "fix: Fix Pipeline variables related customer issues (a…

9229ebd

…ws#2959)" (aws#3041)" This reverts commit 2782f8c.

fix: Include deprecated JsonGet into PipelineVariable

34628be

qidewenwhen force-pushed the dev branch from fe2f885 to 34628be Compare April 7, 2022 19:31

ahsan-z-khan approved these changes Apr 7, 2022

View reviewed changes

ahsan-z-khan merged commit 6ffe925 into aws:master Apr 7, 2022

This was referenced Dec 2, 2022

Fix for #2949. #2951

Closed

EstimatorBase._json_encode_hyperparameters(hyperparams) breaks sagemaker_training.py #2949

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add back the Fix for Pipeline variables related customer issues #3043

fix: Add back the Fix for Pipeline variables related customer issues #3043

qidewenwhen commented Apr 2, 2022

sagemaker-bot commented Apr 2, 2022

codecov-commenter commented Apr 2, 2022 •

edited

Loading

sagemaker-bot commented Apr 2, 2022

sagemaker-bot commented Apr 4, 2022

sagemaker-bot commented Apr 4, 2022

sagemaker-bot commented Apr 5, 2022

sagemaker-bot commented Apr 5, 2022

sagemaker-bot commented Apr 5, 2022

shreyapandit Apr 7, 2022

qidewenwhen Apr 7, 2022 •

edited

Loading

shreyapandit Apr 7, 2022

qidewenwhen Apr 7, 2022

sagemaker-bot commented Apr 7, 2022

sagemaker-bot commented Apr 7, 2022

sagemaker-bot commented Apr 7, 2022

fix: Add back the Fix for Pipeline variables related customer issues #3043

fix: Add back the Fix for Pipeline variables related customer issues #3043

Conversation

qidewenwhen commented Apr 2, 2022

Merge Checklist

General

Tests

sagemaker-bot commented Apr 2, 2022

AWS CodeBuild CI Report

codecov-commenter commented Apr 2, 2022 • edited Loading

Codecov Report

sagemaker-bot commented Apr 2, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Apr 4, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Apr 4, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Apr 5, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Apr 5, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Apr 5, 2022

AWS CodeBuild CI Report

shreyapandit Apr 7, 2022

Choose a reason for hiding this comment

qidewenwhen Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

shreyapandit Apr 7, 2022

Choose a reason for hiding this comment

qidewenwhen Apr 7, 2022

Choose a reason for hiding this comment

sagemaker-bot commented Apr 7, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Apr 7, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Apr 7, 2022

AWS CodeBuild CI Report

codecov-commenter commented Apr 2, 2022 •

edited

Loading

qidewenwhen Apr 7, 2022 •

edited

Loading